Marking a class abstract should not mark the methods abstract as well#23291
Marking a class abstract should not mark the methods abstract as well#23291MisinformedDNA wants to merge 4 commits intoOpenAPITools:masterfrom
Conversation
There was a problem hiding this comment.
No issues found across 4 files
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Add one-off context when rerunning by tagging
@cubic-dev-aiwith guidance or docs links (includingllms.txt) - Ask questions if you need clarification on any suggestion
|
Interesting, and thanks for contributing! ❤️ Could you please generate samples so that the differences are shown here in the diffs? Cheers! |
|
Thanks for the PR but your commit (as shown in the Commits tab) is not linked to your Github account, which means this PR won't count as your contribution in https://github.com/OpenAPITools/openapi-generator/graphs/contributors. Let me know if you need help fixing it. |
|
@wing328 This isn't right?
|
There was a problem hiding this comment.
11 issues found across 183 files (changes from recent commits).
Note: This PR contains a large number of files. cubic only reviews up to 75 files per PR, so some files may not have been reviewed.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="samples/server/petstore/aspnetcore-8.0-abstract-class/src/Org.OpenAPITools/Models/Pet.cs">
<violation number="1" location="samples/server/petstore/aspnetcore-8.0-abstract-class/src/Org.OpenAPITools/Models/Pet.cs:96">
P2: Optional `status` is generated as a non-nullable enum with an invalid default (`0`), so omitted values collapse to an out-of-spec state instead of remaining absent.</violation>
<violation number="2" location="samples/server/petstore/aspnetcore-8.0-abstract-class/src/Org.OpenAPITools/Models/Pet.cs:199">
P1: `Equals` compares list fields structurally, but `GetHashCode` hashes list references, violating equality/hash-code consistency.</violation>
</file>
<file name="samples/server/petstore/aspnetcore-8.0-abstract-class/src/Org.OpenAPITools/Dockerfile">
<violation number="1" location="samples/server/petstore/aspnetcore-8.0-abstract-class/src/Org.OpenAPITools/Dockerfile:4">
P1: Dockerfile references outdated .NET image repo/tags (`dotnet/core` + `8.0-buster*`) that do not exist for .NET 8, risking image pull/build failures.</violation>
</file>
<file name="samples/server/petstore/aspnetcore-8.0-abstract-class/src/Org.OpenAPITools/Controllers/PetApi.cs">
<violation number="1" location="samples/server/petstore/aspnetcore-8.0-abstract-class/src/Org.OpenAPITools/Controllers/PetApi.cs:55">
P1: Default virtual controller methods overwrite JSON examples with XML and then parse with JsonConvert.DeserializeObject, causing runtime failures in reachable default endpoints.</violation>
</file>
<file name="samples/server/petstore/aspnetcore-8.0-abstract-class/src/Org.OpenAPITools/Controllers/StoreApi.cs">
<violation number="1" location="samples/server/petstore/aspnetcore-8.0-abstract-class/src/Org.OpenAPITools/Controllers/StoreApi.cs:102">
P2: `exampleJson` is overwritten with XML and then parsed with `JsonConvert.DeserializeObject<Order>`, creating a format mismatch and runtime-failure path in default virtual method implementations.</violation>
</file>
<file name="samples/server/petstore/aspnetcore-8.0-abstract-class/src/Org.OpenAPITools/Controllers/UserApi.cs">
<violation number="1" location="samples/server/petstore/aspnetcore-8.0-abstract-class/src/Org.OpenAPITools/Controllers/UserApi.cs:138">
P1: `GetUserByName` overwrites JSON example with XML and then JSON-deserializes it, causing runtime failure in default execution path.</violation>
</file>
<file name="samples/server/petstore/aspnetcore-8.0-abstract-class/src/Org.OpenAPITools/Filters/BasePathFilter.cs">
<violation number="1" location="samples/server/petstore/aspnetcore-8.0-abstract-class/src/Org.OpenAPITools/Filters/BasePathFilter.cs:53">
P2: `BasePath` is interpolated directly into a regex pattern, so regex metacharacters can break or alter path-prefix removal.</violation>
</file>
<file name="samples/server/petstore/aspnetcore-8.0-abstract-class/src/Org.OpenAPITools/Filters/GeneratePathParamsValidationFilter.cs">
<violation number="1" location="samples/server/petstore/aspnetcore-8.0-abstract-class/src/Org.OpenAPITools/Filters/GeneratePathParamsValidationFilter.cs:97">
P2: `[Range]` is translated to string length constraints and int-only casts, causing incorrect OpenAPI schema constraints and potential invalid casts for non-int ranges.</violation>
</file>
<file name="samples/server/petstore/aspnetcore-8.0-abstract-class/src/Org.OpenAPITools/Models/Cat.cs">
<violation number="1" location="samples/server/petstore/aspnetcore-8.0-abstract-class/src/Org.OpenAPITools/Models/Cat.cs:79">
P2: `Cat` equality/hash code ignore inherited `Animal` fields, causing incorrect equality semantics and hash collisions for instances that differ in base-class state.</violation>
</file>
<file name="samples/server/petstore/aspnetcore-8.0-abstract-class/src/Org.OpenAPITools/Startup.cs">
<violation number="1" location="samples/server/petstore/aspnetcore-8.0-abstract-class/src/Org.OpenAPITools/Startup.cs:61">
P2: API key documented for sample usage does not match the key enforced by authorization policy, causing documented auth test value to fail.</violation>
<violation number="2" location="samples/server/petstore/aspnetcore-8.0-abstract-class/src/Org.OpenAPITools/Startup.cs:148">
P1: Authorization middleware is missing from the request pipeline, so `[Authorize(Policy = "api_key")]` endpoints are not properly enforced.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| if (Name != null) | ||
| hashCode = hashCode * 59 + Name.GetHashCode(); | ||
| if (PhotoUrls != null) | ||
| hashCode = hashCode * 59 + PhotoUrls.GetHashCode(); |
There was a problem hiding this comment.
P1: Equals compares list fields structurally, but GetHashCode hashes list references, violating equality/hash-code consistency.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At samples/server/petstore/aspnetcore-8.0-abstract-class/src/Org.OpenAPITools/Models/Pet.cs, line 199:
<comment>`Equals` compares list fields structurally, but `GetHashCode` hashes list references, violating equality/hash-code consistency.</comment>
<file context>
@@ -0,0 +1,224 @@
+ if (Name != null)
+ hashCode = hashCode * 59 + Name.GetHashCode();
+ if (PhotoUrls != null)
+ hashCode = hashCode * 59 + PhotoUrls.GetHashCode();
+ if (Tags != null)
+ hashCode = hashCode * 59 + Tags.GetHashCode();
</file context>
samples/server/petstore/aspnetcore-8.0-abstract-class/src/Org.OpenAPITools/Dockerfile
Show resolved
Hide resolved
| @@ -0,0 +1,260 @@ | |||
| /* | |||
There was a problem hiding this comment.
P1: Default virtual controller methods overwrite JSON examples with XML and then parse with JsonConvert.DeserializeObject, causing runtime failures in reachable default endpoints.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At samples/server/petstore/aspnetcore-8.0-abstract-class/src/Org.OpenAPITools/Controllers/PetApi.cs, line 55:
<comment>Default virtual controller methods overwrite JSON examples with XML and then parse with JsonConvert.DeserializeObject, causing runtime failures in reachable default endpoints.</comment>
<file context>
@@ -0,0 +1,260 @@
+ exampleJson = "<Pet>\n <id>123456789</id>\n <Category>\n <id>123456789</id>\n <name>aeiou</name>\n </Category>\n <name>doggie</name>\n <photoUrls>\n <photoUrls>aeiou</photoUrls>\n </photoUrls>\n <tags>\n <Tag>\n <id>123456789</id>\n <name>aeiou</name>\n </Tag>\n </tags>\n <status>aeiou</status>\n</Pet>";
+
+ var example = exampleJson != null
+ ? JsonConvert.DeserializeObject<Pet>(exampleJson)
+ : default;
+ //TODO: Change the data returned
</file context>
...es/server/petstore/aspnetcore-8.0-abstract-class/src/Org.OpenAPITools/Controllers/UserApi.cs
Show resolved
Hide resolved
| //TODO: Or alternatively use the original OpenAPI contract that's included in the static files | ||
| // c.SwaggerEndpoint("/openapi-original.json", "OpenAPI Petstore Original"); | ||
| }); | ||
| app.UseRouting(); |
There was a problem hiding this comment.
P1: Authorization middleware is missing from the request pipeline, so [Authorize(Policy = "api_key")] endpoints are not properly enforced.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At samples/server/petstore/aspnetcore-8.0-abstract-class/src/Org.OpenAPITools/Startup.cs, line 148:
<comment>Authorization middleware is missing from the request pipeline, so `[Authorize(Policy = "api_key")]` endpoints are not properly enforced.</comment>
<file context>
@@ -0,0 +1,155 @@
+ //TODO: Or alternatively use the original OpenAPI contract that's included in the static files
+ // c.SwaggerEndpoint("/openapi-original.json", "OpenAPI Petstore Original");
+ });
+ app.UseRouting();
+ app.UseEndpoints(endpoints =>
+ {
</file context>
| // return StatusCode(404); | ||
| string exampleJson = null; | ||
| exampleJson = "{\r\n \"petId\" : 6,\r\n \"quantity\" : 1,\r\n \"id\" : 0,\r\n \"shipDate\" : \"2000-01-23T04:56:07.000+00:00\",\r\n \"complete\" : false,\r\n \"status\" : \"placed\"\r\n}"; | ||
| exampleJson = "<Order>\n <id>123456789</id>\n <petId>123456789</petId>\n <quantity>123</quantity>\n <shipDate>2000-01-23T04:56:07.000Z</shipDate>\n <status>aeiou</status>\n <complete>true</complete>\n</Order>"; |
There was a problem hiding this comment.
P2: exampleJson is overwritten with XML and then parsed with JsonConvert.DeserializeObject<Order>, creating a format mismatch and runtime-failure path in default virtual method implementations.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At samples/server/petstore/aspnetcore-8.0-abstract-class/src/Org.OpenAPITools/Controllers/StoreApi.cs, line 102:
<comment>`exampleJson` is overwritten with XML and then parsed with `JsonConvert.DeserializeObject<Order>`, creating a format mismatch and runtime-failure path in default virtual method implementations.</comment>
<file context>
@@ -0,0 +1,141 @@
+ // return StatusCode(404);
+ string exampleJson = null;
+ exampleJson = "{\r\n \"petId\" : 6,\r\n \"quantity\" : 1,\r\n \"id\" : 0,\r\n \"shipDate\" : \"2000-01-23T04:56:07.000+00:00\",\r\n \"complete\" : false,\r\n \"status\" : \"placed\"\r\n}";
+ exampleJson = "<Order>\n <id>123456789</id>\n <petId>123456789</petId>\n <quantity>123</quantity>\n <shipDate>2000-01-23T04:56:07.000Z</shipDate>\n <status>aeiou</status>\n <complete>true</complete>\n</Order>";
+
+ var example = exampleJson != null
</file context>
| { | ||
| if (key.StartsWith(BasePath)) | ||
| { | ||
| var newKey = Regex.Replace(key, $"^{BasePath}", string.Empty); |
There was a problem hiding this comment.
P2: BasePath is interpolated directly into a regex pattern, so regex metacharacters can break or alter path-prefix removal.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At samples/server/petstore/aspnetcore-8.0-abstract-class/src/Org.OpenAPITools/Filters/BasePathFilter.cs, line 53:
<comment>`BasePath` is interpolated directly into a regex pattern, so regex metacharacters can break or alter path-prefix removal.</comment>
<file context>
@@ -0,0 +1,60 @@
+ {
+ if (key.StartsWith(BasePath))
+ {
+ var newKey = Regex.Replace(key, $"^{BasePath}", string.Empty);
+ openapiDoc.Paths.Remove(key);
+ openapiDoc.Paths.Add(newKey, value);
</file context>
| var newKey = Regex.Replace(key, $"^{BasePath}", string.Empty); | |
| var newKey = Regex.Replace(key, $"^{Regex.Escape(BasePath)}", string.Empty); |
| var rangeAttr = attributes.FirstOrDefault(p => p.AttributeType == typeof(RangeAttribute)); | ||
| if (rangeAttr != null) | ||
| { | ||
| var rangeMin = (int)rangeAttr.ConstructorArguments[0].Value; |
There was a problem hiding this comment.
P2: [Range] is translated to string length constraints and int-only casts, causing incorrect OpenAPI schema constraints and potential invalid casts for non-int ranges.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At samples/server/petstore/aspnetcore-8.0-abstract-class/src/Org.OpenAPITools/Filters/GeneratePathParamsValidationFilter.cs, line 97:
<comment>`[Range]` is translated to string length constraints and int-only casts, causing incorrect OpenAPI schema constraints and potential invalid casts for non-int ranges.</comment>
<file context>
@@ -0,0 +1,107 @@
+ var rangeAttr = attributes.FirstOrDefault(p => p.AttributeType == typeof(RangeAttribute));
+ if (rangeAttr != null)
+ {
+ var rangeMin = (int)rangeAttr.ConstructorArguments[0].Value;
+ var rangeMax = (int)rangeAttr.ConstructorArguments[1].Value;
+
</file context>
| if (other is null) return false; | ||
| if (ReferenceEquals(this, other)) return true; | ||
|
|
||
| return |
There was a problem hiding this comment.
P2: Cat equality/hash code ignore inherited Animal fields, causing incorrect equality semantics and hash collisions for instances that differ in base-class state.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At samples/server/petstore/aspnetcore-8.0-abstract-class/src/Org.OpenAPITools/Models/Cat.cs, line 79:
<comment>`Cat` equality/hash code ignore inherited `Animal` fields, causing incorrect equality semantics and hash collisions for instances that differ in base-class state.</comment>
<file context>
@@ -0,0 +1,119 @@
+ if (other is null) return false;
+ if (ReferenceEquals(this, other)) return true;
+
+ return
+ (
+ Declawed == other.Declawed ||
</file context>
| authConfig.AddPolicy("api_key", policyBuilder => | ||
| { | ||
| policyBuilder | ||
| .AddRequirements(new ApiKeyRequirement(new[] { "my-secret-key" },"api_key")); |
There was a problem hiding this comment.
P2: API key documented for sample usage does not match the key enforced by authorization policy, causing documented auth test value to fail.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At samples/server/petstore/aspnetcore-8.0-abstract-class/src/Org.OpenAPITools/Startup.cs, line 61:
<comment>API key documented for sample usage does not match the key enforced by authorization policy, causing documented auth test value to fail.</comment>
<file context>
@@ -0,0 +1,155 @@
+ authConfig.AddPolicy("api_key", policyBuilder =>
+ {
+ policyBuilder
+ .AddRequirements(new ApiKeyRequirement(new[] { "my-secret-key" },"api_key"));
+ });
+ });
</file context>
|
go to https://github.com/OpenAPITools/openapi-generator/pull/23291/commits and your ID/name in the commit is not clickable (not linkted to your github account) |
4a45881 to
2d45cca
Compare
Fixed. Thanks! (Somehow, I had the wrong fake email. lol) |
There was a problem hiding this comment.
2 issues found across 16 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="samples/server/petstore/aspnetcore-6.0-useSwashBuckle/src/Org.OpenAPITools/Controllers/UserApi.cs">
<violation number="1" location="samples/server/petstore/aspnetcore-6.0-useSwashBuckle/src/Org.OpenAPITools/Controllers/UserApi.cs:133">
P1: `GetUserByName` overwrites JSON example with XML and then always calls `JsonConvert.DeserializeObject`, causing runtime failure in the new default implementation.</violation>
</file>
<file name="samples/server/petstore/aspnetcore-6.0-useSwashBuckle/src/Org.OpenAPITools/Controllers/PetApi.cs">
<violation number="1" location="samples/server/petstore/aspnetcore-6.0-useSwashBuckle/src/Org.OpenAPITools/Controllers/PetApi.cs:98">
P2: Default virtual controller implementations overwrite JSON with XML and then deserialize via JsonConvert, creating runtime failures when inherited methods are invoked.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| exampleJson = "<User>\n <id>123456789</id>\n <username>aeiou</username>\n <firstName>aeiou</firstName>\n <lastName>aeiou</lastName>\n <email>aeiou</email>\n <password>aeiou</password>\n <phone>aeiou</phone>\n <userStatus>123</userStatus>\n</User>"; | ||
|
|
||
| var example = exampleJson != null | ||
| ? JsonConvert.DeserializeObject<User>(exampleJson) |
There was a problem hiding this comment.
P1: GetUserByName overwrites JSON example with XML and then always calls JsonConvert.DeserializeObject, causing runtime failure in the new default implementation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At samples/server/petstore/aspnetcore-6.0-useSwashBuckle/src/Org.OpenAPITools/Controllers/UserApi.cs, line 133:
<comment>`GetUserByName` overwrites JSON example with XML and then always calls `JsonConvert.DeserializeObject`, causing runtime failure in the new default implementation.</comment>
<file context>
@@ -86,7 +116,25 @@ public abstract class UserApiController : ControllerBase
+ exampleJson = "<User>\n <id>123456789</id>\n <username>aeiou</username>\n <firstName>aeiou</firstName>\n <lastName>aeiou</lastName>\n <email>aeiou</email>\n <password>aeiou</password>\n <phone>aeiou</phone>\n <userStatus>123</userStatus>\n</User>";
+
+ var example = exampleJson != null
+ ? JsonConvert.DeserializeObject<User>(exampleJson)
+ : default;
+ //TODO: Change the data returned
</file context>
| @@ -36,7 +36,23 @@ public abstract class PetApiController : ControllerBase | |||
| [Consumes("application/json", "application/xml")] | |||
There was a problem hiding this comment.
P2: Default virtual controller implementations overwrite JSON with XML and then deserialize via JsonConvert, creating runtime failures when inherited methods are invoked.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At samples/server/petstore/aspnetcore-6.0-useSwashBuckle/src/Org.OpenAPITools/Controllers/PetApi.cs, line 98:
<comment>Default virtual controller implementations overwrite JSON with XML and then deserialize via JsonConvert, creating runtime failures when inherited methods are invoked.</comment>
<file context>
@@ -60,7 +83,23 @@ public abstract class PetApiController : ControllerBase
+ exampleJson = "<Pet>\n <id>123456789</id>\n <Category>\n <id>123456789</id>\n <name>aeiou</name>\n </Category>\n <name>doggie</name>\n <photoUrls>\n <photoUrls>aeiou</photoUrls>\n </photoUrls>\n <tags>\n <Tag>\n <id>123456789</id>\n <name>aeiou</name>\n </Tag>\n </tags>\n <status>aeiou</status>\n</Pet>";
+
+ var example = exampleJson != null
+ ? JsonConvert.DeserializeObject<List<Pet>>(exampleJson)
+ : default;
+ //TODO: Change the data returned
</file context>
|
@Chrimle Ready. |

The code comments say
From a C# perspective, this is not true. This change allows virtual operations in an abstract class. Some unit tests have been added.
PR checklist
Commit all changed files.
This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
These must match the expectations made by your contribution.
You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example
./bin/generate-samples.sh bin/configs/java*.IMPORTANT: Do NOT purge/delete any folders/files (e.g. tests) when regenerating the samples as manually written tests may be removed.
master(upcoming7.x.0minor release - breaking changes with fallbacks),8.0.x(breaking changes without fallbacks)"fixes #123"present in the PR description)@mandrean @shibayan @Blackclaws @lucamazzanti @iBicha
Fixes #23225
Summary by cubic
Allow abstract controller classes without forcing abstract operations in the C# ASP.NET and Azure Functions generators. Methods default to virtual with bodies unless
operationModifieris set toabstract.Bug Fixes
operationModifier=abstractwhenclassModifier=abstractinAspNetServerCodegenandCSharpFunctionsServerCodegen.operationModifier=virtualandgenerateBody=true; only disable bodies whenoperationModifier=abstract.aspnetcore-6.0-useSwashBucklecontrollers now use virtual methods with stubbed bodies.New Features
aspnetcore-8.0-abstract-classsample config and generated sample project to demonstrate abstract controllers.csharp-functions-abstract-classsample config; moved thecsharp-functionssample tosamples/server/petstoreand removed old client sample artifacts.Written for commit 8314d41. Summary will update on new commits.